-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
psp-9256 allow users added to a project to view an acquisition file. #4555
Conversation
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
8 similar comments
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
config.NewConfig<ProjectPersonModel, Entity.PimsProjectPerson>() | ||
.Map(dest => dest.ProjectPersonId, src => src.Id) | ||
.Map(dest => dest.ProjectId, src => src.ProjectId) | ||
.Map(dest => dest.PersonId, src => src.PersonId) | ||
.Map(dest => dest.Person, src => src.Person) | ||
.Inherits<BaseConcurrentModel, Entity.IBaseEntity>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question - I believe our pattern when we map models into EF entities is that we keep the ID but do not map child entities to avoid accidental updates to the children - right?
I see here that we correctly map the PersonId so that we can point to a new existing person but I also see the mapping of .Map(dest => dest.Person, src => src.Person)
- shouldn't we take this out here?
if (isValidId(retrieved.projectId)) { | ||
retrieved.project = await getProjectFunction(retrieved.projectId); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to do this now - but this could would benefit from a Promise.all(...)
approach to fetch the related entities. Right now we wait for each request to complete in sequence. Putting a TODO comment for now would be fine with me
@@ -75,7 +75,7 @@ export class AcquisitionForm implements WithAcquisitionTeam, WithAcquisitionOwne | |||
acquisitionTypeCode: toTypeCodeNullable(this.acquisitionType), | |||
regionCode: toTypeCodeNullable(Number(this.region)), | |||
projectId: isValidId(this.project?.id) ? this.project!.id : null, | |||
productId: this.product !== '' ? Number(this.product) : null, | |||
productId: isValidId(Number(this.product)) ? Number(this.product) : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for doing this!
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
1 similar comment
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
Signed-off-by: devinleighsmith <[email protected]>
e9c7d6a
to
a187d32
Compare
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4555 |
tests are coming in a future PR.